Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Creating the module #1

Merged
merged 17 commits into from
May 6, 2024
Merged

WIP: Creating the module #1

merged 17 commits into from
May 6, 2024

Conversation

cableman
Copy link
Contributor

@cableman cableman commented Apr 25, 2024

Link to ticket

https://leantime.itkdev.dk/?tab=timesheet#/tickets/showTicket/1064

Description

Audit module to log audit message to configurable backed using Drupal's plugin API.

Screenshot of the result

Screenshot from 2024-04-26 13-13-54

Checklist

  • My code passes our static analysis suite.
  • My code passes our continuous integration process.

If your code does not pass all the requirements on the checklist you have to add a comment explaining why this change
should be exempt from the list.

Additional comments or questions

N/A

@cableman cableman force-pushed the develop branch 12 times, most recently from 714da60 to f1a4c8e Compare April 26, 2024 08:12
@cableman cableman force-pushed the develop branch 8 times, most recently from 3a51f97 to 11d06f0 Compare April 26, 2024 11:13
Copy link
Contributor

@jekuaitk jekuaitk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good!

I think we should consider renaming the $line parameter to $message.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
os2web_audit.links.menu.yml Outdated Show resolved Hide resolved
os2web_audit.links.task.yml Outdated Show resolved Hide resolved
os2web_audit.routing.yml Outdated Show resolved Hide resolved
src/Commands/AuditLogDrushCommands.php Show resolved Hide resolved
* Name of the plugin that started the exception.
*
* @return string
* Name of the plugin if given else "Unknown plugin".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just NULL?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because NULL is not human friendly as most exception will to send to system logs

src/Form/PluginSettingsForm.php Outdated Show resolved Hide resolved
src/Plugin/AuditLogger/AuditLoggerInterface.php Outdated Show resolved Hide resolved
foreach ($curlOptions as $option) {
[$key] = explode(' =>', $option);
$key = trim($key);
if (!defined($key)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe im not fully understanding this. Will this not accept any PHP constants? Is it just to filter out some invalid options?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is should check that it is one of: https://www.php.net/manual/en/function.curl-setopt.php

Have tried to update this part.

@cableman cableman requested a review from jekuaitk May 3, 2024 12:13
@cableman cableman merged commit 203d215 into main May 6, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants